Skip to content

editorconfig: don't trim trailing whitespace in tests #144642

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

lolbinarycat
Copy link
Contributor

some test snapshot files require trailing whitespace, and previously manually editing those snapshot files (as is required for run-make tests and some platform-specific tests) in an editor with editorconfig support would cause that whitespace to be removed, causing CI failures like this one

@rustbot
Copy link
Collaborator

rustbot commented Jul 29, 2025

r? @ehuss

rustbot has assigned @ehuss.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot rustbot added A-meta Area: Issues & PRs about the rust-lang/rust repository itself S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 29, 2025
@rustbot
Copy link
Collaborator

rustbot commented Jul 29, 2025

Some changes occurred in src/tools/cargo

cc @ehuss

@rustbot

This comment has been minimized.

@lolbinarycat lolbinarycat force-pushed the editorconfig-no-run-make branch from 9f764bd to 504a2c1 Compare July 29, 2025 17:01
@mati865
Copy link
Member

mati865 commented Jul 31, 2025

How about disabling it only for stdout and stderr files in tests?

@lolbinarycat
Copy link
Contributor Author

@mati865 there's a few run-make tests that use other file extensions, so collecting a comprehensive list might be a bit tricky.

i think more reliable maybe would be disabling it for all tests/ files except those written in rust and js?

@mati865
Copy link
Member

mati865 commented Jul 31, 2025

@lolbinarycat that sounds sensible to me but I'd leave the decision to the reviewer.

@ehuss
Copy link
Contributor

ehuss commented Aug 6, 2025

r? compiler

@rustbot rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Aug 6, 2025
@rustbot rustbot assigned lcnr and unassigned ehuss Aug 6, 2025
@lcnr
Copy link
Contributor

lcnr commented Aug 7, 2025

I would personally also prefer to limit this restriction only to affected tests. I think it's fine for this to be an ad-hoc list of tests where it matters. I would personally love there to be a tidy check which makes sure we keep some //@ editor-no-trim-whitespace attribute in sync with an opt out in the editor config (and some way to bless the editor config file)

But 🤷 I think only listing the effected test in the editor config would also be fine, don't have any strong opinions here

@lolbinarycat
Copy link
Contributor Author

I would personally also prefer to limit this restriction only to affected tests.

There are over 850 affected tests.

@lcnr
Copy link
Contributor

lcnr commented Aug 8, 2025

and these tests rely on their trailing whitespace? as in, how many tests actually intend to have trailing whitespace instead of being cases where we maybe should actually change them to not do so.

I don't want to block a change which prevents some editors from silently messing up your PR for the sake of a larger change, so I'd be fine with merging this for now and opening an issue that we should reenable this for at least ui tests and maybe even have a tidy check for it

@lolbinarycat lolbinarycat force-pushed the editorconfig-no-run-make branch from 504a2c1 to cad16c3 Compare August 8, 2025 16:07
@lolbinarycat lolbinarycat force-pushed the editorconfig-no-run-make branch from cad16c3 to 7af87d1 Compare August 8, 2025 16:15
@lolbinarycat
Copy link
Contributor Author

and these tests rely on their trailing whitespace? as in, how many tests actually intend to have trailing whitespace instead of being cases where we maybe should actually change them to not do so.

There's also 4381 tests that end with multiple newlines, something that has an actual affect on UX, and something that is also (at least in emacs) also controlled by the trim_trailing_whitespace option.

I re-enabled trim_trailing_whitespace for source files in tests/, except for the two tests that intentionally have trailing whitespace. Hopefully this should be good enough to merge now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-meta Area: Issues & PRs about the rust-lang/rust repository itself S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants